Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Run Cypress on Rust crypto #11828

Merged
merged 3 commits into from
Nov 6, 2023
Merged

Run Cypress on Rust crypto #11828

merged 3 commits into from
Nov 6, 2023

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Nov 3, 2023

Now that we have enabled Element-R on develop.element.io, it is particularly important that it keeps working. Let's try running Cypress on both crypto stacks.


This change is marked as an internal change (Task), so will not be included in the changelog.

@richvdh richvdh requested a review from a team as a code owner November 3, 2023 15:48
@richvdh richvdh requested a review from t3chguy November 3, 2023 15:48
@richvdh richvdh added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Nov 3, 2023
richvdh added a commit to matrix-org/matrix-js-sdk that referenced this pull request Nov 3, 2023
matrix-org/matrix-react-sdk#11828 removes the
`rust-crypto` input param from the reusable cypress workflow. This gets rid of
it on the calling side.
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will end up mangling results from both into one artifact in Kiwi which will likely yield undefined behaviour due to all test fixtures being listed twice, no? And even in the debugging cypress-results artifact they'd clobber each other

@richvdh
Copy link
Member Author

richvdh commented Nov 6, 2023

I don't think I've ever managed to get access to Kiwi, so I don't really know how it works, but I guess you're right. You're definitely right about the results artifact, anyway.

Have done some stuff which seems like it might be plausible, but also requesting review from @michaelkaye since this touches Kiwi stuff.

@@ -84,7 +80,7 @@ jobs:
core.setOutput("email", response.data.author.email);

# Only run Percy when it is demanded or we are running the daily build
- name: Enable Percy if X-Needs-Percy
- name: Enable Percy
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incidentally: this "if X-Needs-Percy" was just a lie. Removed it as I don't think there is any particular benefit to repeating the exact conditions under which the step will run: as demonstrated, it is just a thing that we fail to maintain.

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loving the emoji

Copy link
Contributor

@michaelkaye michaelkaye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine but maybe using the build id will define the relationship better - it's the same source code hash but built differently.

@@ -289,5 +292,5 @@ jobs:
product: "Element Web"
product-version: ${{ github.event.workflow_run.head_branch }}
build-id: ${{ github.event.workflow_run.head_sha }}
suite-name: "Cypress E2E"
suite-name: "Cypress E2E (${{ matrix.crypto }} crypto)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So actually i think we want this chnge on the build-id rather than the suite name; that would get us the same suite running against two versions (with/without rust) rather than two separate suites. Something like

Both suites will have the same tests in them so that shouldn't be a problem to have two builds going on here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelkaye thanks. something like this? (eeffc8a)

... instead of test suite name
@richvdh richvdh added this pull request to the merge queue Nov 6, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 6, 2023
@richvdh richvdh added this pull request to the merge queue Nov 6, 2023
Merged via the queue into develop with commit c344a37 Nov 6, 2023
22 checks passed
@richvdh richvdh deleted the rav/cypress-element-r branch November 6, 2023 17:18
github-merge-queue bot pushed a commit to matrix-org/matrix-js-sdk that referenced this pull request Nov 9, 2023
matrix-org/matrix-react-sdk#11828 removes the
`rust-crypto` input param from the reusable cypress workflow. This gets rid of
it on the calling side.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants